Skip to content

Conversation

geo2a
Copy link
Contributor

@geo2a geo2a commented Dec 20, 2023

This is a revert of the revert of #3704. Needs to be merged in sync with runtimeverification/hs-backend-booster#427 and also a change to pyk to handle the new modules IDs.

@goodlyrottenapple goodlyrottenapple changed the title Concurrent kore-rpc-booster strikes back Concurrent kore-rpc strikes back Jan 8, 2024
@tothtamas28
Copy link
Contributor

Could you also update the docs according to these changes, please?

@tothtamas28
Copy link
Contributor

I opened a PR for the necessary changes in pyk: runtimeverification/pyk#808.

I also added a few integration tests, and one of them is failing: if you add a module, then add the same module with "name-as-id": true, then execute with the module name, the server responds with error "Could not find module".

Running the tests: ensure the right K version is on PATH, then from the pyk repo root execute

$ make test-integration TEST_ARGS='-n0 -k TestAddModule'

Copy link
Member

@jberthold jberthold left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code LGTM
(:+1: new code does what we wanted)

Pre-approving, but let's add add-module/add-again to the test script.

Comment on lines 31 to 32
Expected:
* Module successfully added even though it has been added already, because the content is exactly the same.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably the module would be added twice then?
How is add-module/add-again executed? The test script does not mention it.

@goodlyrottenapple goodlyrottenapple merged commit de63565 into master Jan 24, 2024
@goodlyrottenapple goodlyrottenapple deleted the sam/concurrent-kore branch January 24, 2024 09:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants